Skip to content

Implement some more checks in ptr_guaranteed_cmp. #144885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Aug 4, 2025

  • Pointers with different residues modulo their allocations' least common alignment are never equal.
  • Pointers to the same static allocation are equal if and only if they have the same offset.
  • Pointers to different non-zero-sized static allocations are unequal if both point within their allocation, and not on opposite ends.

Tracking issue for const_raw_ptr_comparison: #53020

This should not affect is_null, the only usage of this intrinsic on stable.

Closes #144584

Pointers with different residues modulo their least common allocation alignment are never equal.
Pointers to the same static allocation are equal if and only if they have the same offset.
Pointers to different non-zero-sized static allocations are unequal if both point within their allocation, and not on opposite ends.
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 4, 2025
@theemathas
Copy link
Contributor

Are two different immutable statics required to have distinct addresses? In other words, is the compiler allowed to optimize two immutable statics with identical contents to be stored at the same place?

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2025

I don't think it is, but this also restricts people doing their own custom linking shenanigans, so we should have some stable docs to point to before having logic of this sort in const-eval. That sounds like something t-lang should sign off on.

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2025

If you just want to fix #144584, please reduce the PR to only affect the case where both pointers are based on the same static.

@zachs18
Copy link
Contributor Author

zachs18 commented Aug 4, 2025

Are two different immutable statics required to have distinct addresses? In other words, is the compiler allowed to optimize two immutable statics with identical contents to be stored at the same place?

Yes, they are required to have distinct addresses (if non-zero-sized). No, the compiler is not allowed to coalesce two immutable statics with the same contents.

we should have some stable docs to point to before having logic of this sort in const-eval.

This is guaranteed for non-zero-sized statics, in the Reference:

https://doc.rust-lang.org/nightly/reference/items/static-items.html#r-items.static.storage-disjointness

If the static has a size of at least 1 byte, this allocated object is disjoint from all other such static objects as well as heap allocations and stack-allocated variables. However, the storage of immutable static items can overlap with objects that do not themselves have a unique address, such as promoteds and const items.

This (non-zero-sized statics not overlapping) has been FCP'd by T-lang here (though not anything about consteval specifically)

@davidtwco
Copy link
Member

r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned davidtwco Aug 13, 2025
@zachs18
Copy link
Contributor Author

zachs18 commented Aug 17, 2025

If you just want to fix #144584, please reduce the PR to only affect the case where both pointers are based on the same static.

I would prefer to land all of these changes, but if "Pointers to the same static allocation are equal if and only if they have the same offset" is more straightforward to land than the other two, then I can split them into a followup PR.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an impressive test, thanks a lot for writing it!

// runtime inequality of pointers to different ones) (see e.g. #73722).
Some(GlobalAlloc::Function { .. } | GlobalAlloc::VTable(..)) => 2,
// FIXME: Can these be duplicated (or deduplicated)?
Some(GlobalAlloc::Memory(..) | GlobalAlloc::TypeId { .. }) => 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently Memory definitely can be deduplicated. TypeId is its own thing that exists mostly to prevent const-eval from comparing it so we should definitely always return 2 there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll split and update the comment for TypeId here.

I'll update the comment in the different-AllocId branch to state that Memory can be deduplicated (and Vtable and Function too).

Can Memory be duplicated? That is, should two pointers to the same Memory allocation (at the same offset) compare equal, or be uncomparable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can Memory be duplicated? That is, should two pointers to the same Memory allocation (at the same offset) compare equal, or be uncomparable?

Until we fix #128775, yes that can happen.

`GlobalAlloc::TypeId` exists mostly to prevent consteval from comparing `TypeId`s, so always
return 2 for those (whether the pointers are to the same allocation or not).
In the different-allocation case, document that `GlobalAlloc::Memory`, `Function`, and `Vtable` can
be deduplicated, at least with other allocations of the same kind, so those should return 2.
When comparing a pointer with an integer(-valued pointer), if the integer and the pointer have a different residue
modulo the pointer's allocation's alignment, they can never be equal. Else, we can't know.
@zachs18
Copy link
Contributor Author

zachs18 commented Aug 19, 2025

Latest push addresses some parts of review comments, moves the test to tests/ui/consts (combining and overwriting the existing test for guaranteed_cmp there), and adds alignment checks when comparing pointers with integer-valued pointers.

@RalfJung
Copy link
Member

and adds alignment checks when comparing pointers with integer-valued pointers.

I would have preferred if you didn't extend the scope of the PR after I already did most of my review. Now I'll have to re-review, so back on the queue it goes.

@zachs18
Copy link
Contributor Author

zachs18 commented Aug 19, 2025

and adds alignment checks when comparing pointers with integer-valued pointers.

I would have preferred if you didn't extend the scope of the PR after I already did most of my review. Now I'll have to re-review, so back on the queue it goes.

This is a separate commit. If you prefer I can split this into a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointer to static is not guaranteed_eq to itself.
5 participants